Skip to content

fix: add rollback semantics to secure-storage credential operations#241

Draft
bukinoshita wants to merge 1 commit intomainfrom
fix/auth-secure-storage-desync-4109
Draft

fix: add rollback semantics to secure-storage credential operations#241
bukinoshita wants to merge 1 commit intomainfrom
fix/auth-secure-storage-desync-4109

Conversation

@bukinoshita
Copy link
Copy Markdown
Member

@bukinoshita bukinoshita commented Apr 9, 2026

Summary

Resolves BU-632: CLI auth secure-storage desync strands profiles and credentials.

The secure-storage login/logout paths in src/lib/config.ts updated the OS credential backend and credentials.json in separate steps with no rollback or result verification. A failed write/delete could leave local auth state inconsistent — profiles could remain listed without a usable key, or logout could report success while the secret was still retained in the OS keychain.

Changes

src/lib/config.ts

storeApiKeyAsync — If writeCredentials() throws after backend.set() succeeds, the just-written backend secret is cleaned up via backend.delete() before the error is re-thrown.

removeApiKeyAsync — The key is read from the backend before deletion. backend.delete() return value is now checked; a false return is treated as a hard failure. If the subsequent file mutation (removeApiKey) fails, the key is restored in the backend.

removeAllApiKeysAsync — All backend.delete() results are aggregated using Promise.all with individual result tracking. The credentials file is only removed when every backend delete succeeds. Partial failures throw with a descriptive error listing the failed profiles.

renameProfileAsyncbackend.delete() return value is verified. If the old-name delete fails, the new-name entry is cleaned up. If the file-level renameProfile() fails after successful backend operations, the backend state is restored (old key re-set, new key deleted).

tests/lib/config-async.test.ts

  • Updated existing tests to use it() instead of test() per coding standards
  • Updated mock delete return values from undefined to true to match the new verification behavior
  • Added 5 new tests covering the failure/rollback scenarios:
    • storeApiKeyAsync: backend rollback on file write failure
    • removeApiKeyAsync: throws on false delete return; restores key on file failure
    • removeAllApiKeysAsync: preserves credentials file when any backend delete fails
    • renameProfileAsync: cleans up new-name on old-name delete failure; restores old-name on file write failure

Verification

  • All 741 tests pass (90 test files)
  • Biome lint/format clean
  • TypeScript typecheck passes

Linear Issue: BU-632

Open in Web Open in Cursor 

Summary by cubic

Adds transactional rollback to secure-storage credential operations so the OS keychain and credentials.json stay in sync. Fixes BU-632 by preventing stranded profiles and orphaned secrets on partial failures.

  • Bug Fixes
    • storeApiKeyAsync: roll back backend.set() if writing credentials.json fails.
    • removeApiKeyAsync: require backend.delete() to return true; restore the key if file removal fails.
    • removeAllApiKeysAsync: remove the file only if all backend deletes succeed; throw with failed profile names otherwise.
    • renameProfileAsync: verify old-name delete; clean up new-name or restore old-name if file rename fails.

Written for commit 10df862. Summary will update on new commits.

Resolve data integrity issue (BU-632) where secure-storage login/logout
paths could leave local auth state inconsistent when a backend or
filesystem failure occurs mid-operation.

Changes to src/lib/config.ts:
- storeApiKeyAsync: roll back backend.set() if credentials file write fails
- removeApiKeyAsync: verify backend.delete() returns true before proceeding;
  restore the key in the backend if file removal fails afterward
- removeAllApiKeysAsync: aggregate all delete results and abort with error
  if any backend deletion fails, preserving the credentials file
- renameProfileAsync: verify backend.delete() returns true; roll back
  backend state if either the old-name delete fails or the file rename
  fails

Tests added for each failure mode:
- storeApiKeyAsync: backend rollback on file write failure
- removeApiKeyAsync: throws on false delete return; restores key on
  file failure
- removeAllApiKeysAsync: preserves file when any backend delete fails
- renameProfileAsync: cleans up new-name on old-name delete failure;
  restores old-name on file write failure

Co-authored-by: Bu Kinoshita <bukinoshita@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants